Use restored runtime sessions in notebooks and improve notebook runtime startup/shutdown stability #2604
Conversation
| const startPromise = new DeferredPromise<positron.LanguageRuntimeSession>(); | ||
| this._startingSessionsByNotebookUri.set(notebookUri, startPromise); | ||
|
|
||
| // If the notebook has a session that is still shutting down, wait for it to finish. |
There was a problem hiding this comment.
This block is new, to improve concurrent attempts to start/shutdown.
| throw err; | ||
| } | ||
|
|
||
| // If there's already a session for this runtime e.g. one restored after a window reload, use it. |
There was a problem hiding this comment.
This block is new, to try to use a restored session after a window reload.
| } | ||
|
|
||
| async shutdownRuntimeSession(notebookUri: Uri): Promise<void> { | ||
| // Return the existing promise, if there is one. |
There was a problem hiding this comment.
Tracking the shutdown promise is new, to improve concurrent attempts to start/shutdown.
| throw err; | ||
| } | ||
|
|
||
| // Wait for the session to end. This is necessary so that we know when to start the next |
There was a problem hiding this comment.
Waiting for shutdown to complete is new, to improve concurrent attempts to start/shutdown.
| * A map of sessions currently starting, keyed by notebook URI. Values are promises that resolve | ||
| * when the session has started and is ready to execute code. | ||
| */ | ||
| private readonly _startingSessionsByNotebookUri = new ResourceMap<DeferredPromise<positron.LanguageRuntimeSession>>(); |
There was a problem hiding this comment.
These are now ResourceMaps that key on the URI instead of the notebook document. The latter was not stable across all cases that we needed it to be.
| * runtime sessions; it manages the set of active sessions and provides | ||
| * facilities for starting, stopping, and interacting with them. | ||
| * | ||
| * TODO(seem): The intention is for functionality here to eventually be brought into Positron core. |
There was a problem hiding this comment.
Most of this looks like it's copied wholesale from the runtime session service, which is fine for now! But you might want to leave a comment indicating as such.
It's worth noting that the R extension has also grown something like this to keep track of sessions it started: https://github.com/posit-dev/positron/blob/main/extensions/positron-r/src/session-manager.ts
This is a code smell; it really shouldn't be necessary for extensions to do this. I think we need more API exposure to the set of active sessions so that extensions are less incented to do it themselves. No need to try to tackle that in your PR but let's file an issue to address holistically.
There was a problem hiding this comment.
Agreed! I've created the currently empty issue #2620 to track this. I'll put together a proposal over there and let you know once it's ready.
extensions/positron-notebook-controllers/src/notebookSessionService.ts
Outdated
Show resolved
Hide resolved
| // If there's already a session for this runtime e.g. one restored after a window reload, use it. | ||
| let session: positron.LanguageRuntimeSession | undefined; | ||
| try { | ||
| session = await positron.runtime.getNotebookSession(notebookUri); |
There was a problem hiding this comment.
Do we need to check to make sure the session is in a good state (i.e. hasn't exited) before we reuse it?
There was a problem hiding this comment.
I've added TODOs for now. I haven't encountered this in my testing, and I think it becomes easier after some API improvements, so will defer for now.
extensions/positron-notebook-controllers/src/notebookController.ts
Outdated
Show resolved
Hide resolved
extensions/positron-notebook-controllers/src/notebookController.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Nick Strayer <[email protected]> Signed-off-by: Wasim Lorgat <[email protected]>
Intent
This PRs fixes restoring the notebook's session on a window reload (#2602) and improves the stability issues around starting/shutting down notebooks (#2603).
Approach
The major change required here is to extract session management out into a new
NotebookSessionServiceclass. Managing sessions in a central place makes it much easier to guarantee that only one session exists for a notebook at any given time, which fixes bugs related to starting/shutting down notebook sessions in quick succession.To use the restored notebook sessions, I added
getNotebookSessionto the API, and track notebook sessions inside the runtime session service. I also needed to "revive" the notebook URIs after deserializing them, else they don't seem to match.We now also track shutting down sessions and ensure that any existing session for a notebook is shutdown before we start a new one.
Also turned out that we were disposing sessions in the controller, but I don't think we should have been, so removed that.
QA Notes
See the linked issues for verification steps. Playing around with opening and closing notebooks, and switching selected kernels, would also be helpful.